FortiOS: update logic in the initial playbook#3336
Conversation
- Task "Enable multi-VDOM mode if a traffic VDOM is defined" is always executed and it set only the proper vdom-mode. - Task "Wait after VDOM mode change" is executed when netlab_vdom_timer > 0 is set - Creation of the traffic VDOM is offloaded to the template
0fe8965 to
f5372a2
Compare
|
Thanks for this, it's looking great — and nice tidy-up on the hostname part. I did a quick test, using a vagrant box on 7.4.8, created without VDOM, just the bare minimum configuration. Test 1 - no
|
I would suggest adding something along these lines to the failing task: We use the same trick in https://github.com/ipspace/netlab/blob/dev/netsim/ansible/tasks/readiness-check/ssh.yml The playbook continues immediately if the task succeeds; otherwise, it retries for however long we specify. This would also make Fortinet implementation similar to others (and using the same retry/delay variables). |
ipspace
left a comment
There was a problem hiding this comment.
A bunch of minor nits. At the very minimum, we should optimize the wait loops.
Also, the configuration-related comments are pure guesswork; I'm afraid I'm missing some nuances.
| sleep: 10 # time in seconds between checks | ||
| delay: 60 # Initial delay in seconds before first check | ||
| sleep: 10 # time in seconds between checks | ||
| delay: "{{ netlab_vdom_timer }}" # Initial delay in seconds before first check |
There was a problem hiding this comment.
This should be replaced with until/delay/retries parameters (see my response to @sdargoeuves comment)
There was a problem hiding this comment.
To my understanding the key part was to sit there quietly for delay seconds first. With until it will hit with http query immediately. But I might be missing something.
| system_global: | ||
| hostname: '{{ inventory_hostname.replace("_","-") }}' | ||
| register: hostname_result | ||
| retries: 5 |
There was a problem hiding this comment.
We could use the same netlab_check_... variables here for consistency.
| retries: 5 | ||
| delay: 10 # Initial delay and waiting time between retries | ||
| delay: 10 # waiting time between retries | ||
| until: hostname_result is not failed and hostname_result.meta.http_status == 200 |
There was a problem hiding this comment.
Would simple hostname_result|success work here?
| delay: 10 # waiting time between retries | ||
| until: hostname_result is not failed and hostname_result.meta.http_status == 200 | ||
| when: >- | ||
| vdom_mode_result.meta is defined and |
There was a problem hiding this comment.
If I understand it correctly, this applies the hostname only when we switched into multi-VDOM mode before, but isn't the hostname applied in the configuration template? I'm probably missing some nuance here.
Also, if the purpose of this task is to check the API readiness (and setting the hostname is just a convoluted way of doing that), then maybe we could use a simpler R/O call and have it executed all the time (without the "when" condition)?
There was a problem hiding this comment.
One more thought: if the "when" condition is identifying whether we changed the VDOM, maybe we can just put the two waiting tasks in a block and apply the when condition to the block? That would skip them completely for @a-v-popov setup while still allowing @sdargoeuves setup to work.
|
One other thought: Do we have to check HTTPS and API readiness? Is there a point where the HTTPS call succeeds but the API is not ready yet? |
|
I think I know what the problem is now. Re hostname, I was trying to keep the guard logic before. But if I understand correctly there is nothing special about hostname specifically. I think we should just remove it from playbook and let it be assigned in the template. I will update the PR. UPDATE: or maybe not https://community.fortinet.com/t5/Support-Forum/429-Status-code-with-message-quot-Too-many-Request-quot/m-p/397611 |
I am not 100% sure about the failure scenario, I suspect some interaction between Ansible FortiOS module frantically trying to do its magic and FortiOS trying to protect itself from DoS attack. API key should short circuit the whole thing. I pushed a second draft. It switches to HTTPS to reduce number of connections. Keeps |
- generate api-user key via ssh - store it in host_vars/<node>/auth.yml - remove hostname from initial playbook, assigned in the template - switch to HTTPS port 443
12f58c3 to
c0b971a
Compare
| "password": "{{ ansible_ssh_pass }}" | ||
| "#": | ||
| - config global | ||
| - execute api-user generate-key netlab |
There was a problem hiding this comment.
This line fails as there is no api user netlab when I tested:
"stdout_lines": [
"Warning: Permanently added '10.194.59.11' (ECDSA) to the list of known hosts.",
"",
"FGVMxxxxxY5C # config global",
"",
"",
"command parse error before 'global'",
"Command fail. Return code 1",
"",
"FGVMxxxxxY5C # execute api-user generate-key netlab",
"",
"Could not find api-user netlab.",
"Command fail. Return code -3",
"",
"FGVMxxxxxY5C # exit",
"",
"Connection to 10.194.59.11 closed."
]
}If I manually configure the API user like this, it then works:
config system api-user
edit "netlab"
set accprofile "super_admin"
next
end
And also (I would need to test further without having to do this step manually) it seems that I no longer need to wait 60s when configuring multi-vdom mode.
It's looking very good, we just need to add the api-user. Is this something you have built in as part of your vagrant box?
There was a problem hiding this comment.
Is this something you have built in as part of your vagrant box?
It should be part of vagrant-box if it was built with cloud-init image.
https://github.com/a-v-popov/netlab/blob/205a09a6cde536a3adbe2b781ae1bfb4ccaaeea5/netsim/install/libvirt/fortios/openstack/latest/user_data#L28
There was a problem hiding this comment.
ah, my issue is that I haven't used the cloud-init image to create my vagrant boxes.
I'd be keen to add the creation of the api-user netlab as part of the task
|
I like where this is going, and creating an API user as part of the initial configuration definitely makes sense. However, once the two of you agree that the new approach is ready to be used, we need to update the caveats documentation to reflect the new reality. It would also be nice to add information about the minimum FortiOS release that's supposed to work, and if you decide that FortiOS 7.0 is too ancient (I have no problem with that), that has to be documented. |
- refactored changes to initial template to allign with a standard mutli-vdom ForitOS configuration
6d3228e to
f78f89f
Compare
|
I decided to curb previous optimization in template in favor of alignment with FortiOS saved configuration structure and general readability. Commits should be squashed to cancel out the mess. I am leaving netlab_vdom_timer default to 0, to let anyone immediately benefit from the change. netlab_generate_api_token: false
netlab_vdom_timer: 60
ansible_httpapi_use_ssl: false
ansible_httpapi_port: 80Let me know if this is not to your liking. I tried to incorporate all the feedback, but please let me know if I missed anything, and/or feel free to amend anything you want. |
|
@ipspace I was able to start a topology with your favorite 7.0(.19) with the setting mentioned above with no-vdom, as it seems multi-vdom was not allowed without a proper license on that release. |
| "password": "{{ ansible_ssh_pass }}" | ||
| "#": | ||
| - config global | ||
| - execute api-user generate-key netlab |
There was a problem hiding this comment.
ah, my issue is that I haven't used the cloud-init image to create my vagrant boxes.
I'd be keen to add the creation of the api-user netlab as part of the task
| responses: | ||
| "password": "{{ ansible_ssh_pass }}" | ||
| "#": | ||
| - config global | ||
| - execute api-user generate-key netlab | ||
| - exit |
There was a problem hiding this comment.
responses:
"password": "{{ ansible_ssh_pass }}"
"#":
- config system api-user
- edit "netlab"
- set accprofile "super_admin"
- next
- end
- config global
- execute api-user generate-key netlab
- exitI've just tested with this, and I've got the famous: it works for me!
There was a problem hiding this comment.
Logically I do not see much difference between admin user and netlab api-user. If one is set in a vagrant box the other could be configured at the same time. Screen scraping works, but it has its drawbacks. On the other end of the spectrum one could prebuild the whole config including both both users and feed it via cloud-image along with a license. Or use admin-scp. But in any case API token is sort of a problem, because it is expendable.
In my setup API keys are generated by a licensing script, so intend to set netlab_generate_api_token: false for my workloads anyway.
- add api-user via SSH - reuse netlab_check_*
|
Was directed to Native Python Types when my calculations failed. |
|
@a-v-popov this is looking great, thank you so much! I've ran a few more tests, including one using a # netlab_vdom: "netlab"
ansible_httpapi_use_ssl: false
ansible_httpapi_port: 80The other two variables don't seem to be required with this specific version of fortios. Other tests involved I think this is great improvement, thank you again! |
|
It's so nice to see things work, but I need two things to get this merged (seeing that we're in "ready for review" stage):
|
I'm not sure about the minimum release we are ok with. I have
Pretty much! I'm OK merging this as long as we agree on the defaults for the new variables, which currently would introduce a breaking change for those older versions ( If we use the values below as the defaults, we might avoid a breaking change based on the versions I have tested: ansible_httpapi_use_ssl: false
ansible_httpapi_port: 80How do you see this part @ipspace @a-v-popov ? |
7.0 is worth mentioning only because it still uses the old licensing model. From that perspective, we could retain the current "7.0" versus "7.4+" split. Obviously, based on one of the @a-v-popov comments, I'd drop 6.x.
I'm perfectly fine with having the default settings that work well with 7.4+ and a caveat saying "you need to adjust these for 7.0", documenting the whole thing as a breaking change.
The real question is thus "do we want to enforce HTTPS?" Is there an advantage to doing that? If not, I'd stick with HTTP and document how to switch over to HTTPS in the caveats. What would you prefer @a-v-popov ? |
The reason I choose HTTPS is that on 7.6 I saw HTTP simply responding with a redirect to HTTPS. Personally, I don't care much, as I need to have
No idea, API keys should work across the board including 6.4/7.0.
|
I'm OK with HTTPS as the default.
Added a caveat and a breaking change description. |
Fix for #3335
netlab_vdom_timer would be 0, if not set. For backward compatibility it might be better choice to set it to 60.
Jinja template is optimized to reduce flow control after VDOM, but I am not sure if
system interfacesstanza has always been available from a VDOM.